Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix reentrancy invariant #39

Merged
merged 12 commits into from
Apr 24, 2024
Merged

fix reentrancy invariant #39

merged 12 commits into from
Apr 24, 2024

Conversation

guybedford
Copy link
Collaborator

@guybedford guybedford commented Apr 2, 2024

This implements an approach to the reentrancy invariant described in #38, in implementing a ValidateSyncExecution routine that runs before the sync evaluation call and checks for any states in the graph associated with reentrancy to skip sync execution in that case.

This would permit the case as described in #38 since it is dealing with disjoint graphs. It is only when the unexecuted parts of the graph overlaps with an in-progress execution that execution would be skipped.

An alternative to skipping execution could still be to throw a reference error.

@nicolo-ribaudo nicolo-ribaudo mentioned this pull request Apr 2, 2024
2 tasks
spec.emu Outdated Show resolved Hide resolved
nicolo-ribaudo
nicolo-ribaudo previously approved these changes Apr 2, 2024
Copy link
Member

@nicolo-ribaudo nicolo-ribaudo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me -- I consider it as fixing a spec bug. I can add a slide asking in plenary if we would prefer to throw in this case.

spec.emu Outdated Show resolved Hide resolved
Co-authored-by: Nicolò Ribaudo <[email protected]>
@guybedford guybedford changed the title normative: handle reentrancy invariant fix reentrancy invariant Apr 2, 2024
@guybedford guybedford mentioned this pull request Apr 2, 2024
10 tasks
@@ -74,7 +74,9 @@ contributors: Nicolò Ribaudo
1. If _P_ is a Symbol, then
1. Return ! OrdinaryGet(_O_, _P_, _Receiver_).
1. <ins>Let _m_ be _O_.[[Module]].</ins>
1. <ins>If _m_ is not a Cyclic Module Record, or both _m_.[[Status]] is ~linked~ and AnyDependencyNeedsAsyncEvaluation(_m_) is *false*, then</ins>
1. <ins>If _m_ is not a Cyclic Module Record, then</ins>
1. <ins>Perform ? EvaluateSync(_m_).</ins>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

EvaluateSync unconditionally calls module.Evaluate(). It will cause the target non Cyclic Module Record to be evaluated every time its namespace object is accessed.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would recommend posting this as a new issue as it is not specific to this PR.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Evaluate() can be called multiple times and it's expected to cache it's result:

Returns a promise for the evaluation of this module and its dependencies, resolving on successful evaluation or if it has already been evaluated successfully, and rejecting for an evaluation error or if it has already been evaluated unsuccessfully.

https://tc39.es/ecma262/multipage/ecmascript-language-scripts-and-modules.html#table-abstract-methods-of-module-records

Dynamic import also calls Evaluate() every time, even if a module is already evaluated.

1. <ins>If _m_ is not a Cyclic Module Record, or both _m_.[[Status]] is ~linked~ and AnyDependencyNeedsAsyncEvaluation(_m_) is *false*, then</ins>
1. <ins>If _m_ is not a Cyclic Module Record, then</ins>
1. <ins>Perform ? EvaluateSync(_m_).</ins>
1. <ins>Otherwise if _m_.[[Status]] is not ~evaluated~ and ! ReadyForSyncExecution(_m_) is *true*, then</ins>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if ReadyForSyncExecution(m) is false, then what will this AO return? The (possibly) uninitialized binding and lead to TDZ error (for export let)?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, in the case of it not being ready, the namespace will expose TDZ or undefined for non-TDZ bindings, per the note.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is also what happens with cycles without defer:

// entrypoint
import "dep";
export let a = 1;
// dep
import * as ns from "entrypoint";
ns.a; // TDZ error

spec.emu Outdated Show resolved Hide resolved
Copy link
Member

@nicolo-ribaudo nicolo-ribaudo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good -- I'll wait before merging it to not have changes merged right before plenary.

spec.emu Outdated Show resolved Hide resolved
Co-authored-by: Nicolò Ribaudo <[email protected]>
@nicolo-ribaudo nicolo-ribaudo merged commit f69b29d into tc39:main Apr 24, 2024
nicolo-ribaudo added a commit to nicolo-ribaudo/proposal-defer-import-eval that referenced this pull request Apr 24, 2024
This commit changes the behavior of namespaces obtained through
`import defer` to re-throw evaluation errors even if the module
is fully evaluated:

```js
import defer * as ns from "module-that-throws";
try { ns.foo } catch { console.log("Error!") }
try { ns.foo } catch { console.log("Error!") }
```

The code above is now guaranteed to print `"Error!"` twice,
and it doesn't depend on whether other modules already evaluated
`"module-that-throws"`.

This is different from the existing behavior of namespace objects,
so we have to use a _different_ namespace object for deferred import:

```js
import defer * as ns1 from "module-that-throws";
import * as ns2 from "module-that-throws";

Promise.resolve().then(() => {
  try { ns1.foo } catch { console.log("Error1!") }
  try { ns2.foo } catch { console.log("Error2!") }
});
```

Assuming that this module's body is evaluated due to
a cycle, the code above will print `"Error1!"` and not
`"Error2!"`. This means that `ns1` !== `ns2`.
Namespace objects are still cached, so if you `import defer`
the same module twice you will get the same namespace object.

Thanks to this change, we can now also make "trying to evaluate
a deferred module in a cycle" an error, rather than silently
skipping its evaluation as done in tc39#39. This ensures that a binding
cannot be accessed _during evaluation_ to then disappear as soon
as the module is done evaluating with an error.

Fixes tc39#41 cc @guybedford
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants